Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[cli] Make chunk size configurable for chunked publish #15427

Merged
merged 3 commits into from
Nov 29, 2024
Merged

[cli] Make chunk size configurable for chunked publish #15427

merged 3 commits into from
Nov 29, 2024
+34 −12

Conversation

0xjunha
Copy link
Contributor

@0xjunha 0xjunha commented Nov 29, 2024

Description

Updated the chunked publish mode in the CLI to include an option for configuring the chunk size.

This gives more flexibility since the chunked publish mode utilizes nearly the maximum payload size permitted for a single transaction. This change helps to handle failures when gas charges are updated.

  • Added a new chunk_size option to ChunkedPublishOption
  • Updated MAX_CHUNK_SIZE_IN_BYTES to be of type &str with a value of 55000

How Has This Been Tested?

  • e2e-move-test
  • manually building & testing the CLI

Key Areas to Review

Type of Change

  • New feature
  • Bug fix
  • Breaking change
  • Performance improvement
  • Refactoring
  • Dependency update
  • Documentation update
  • Tests

Which Components or Systems Does This Change Impact?

  • Validator Node
  • Full Node (API, Indexer, etc.)
  • Move/Aptos Virtual Machine
  • Aptos Framework
  • Aptos CLI/SDK
  • Developer Infrastructure
  • Move Compiler
  • Other (specify)

Checklist

  • I have read and followed the CONTRIBUTING doc
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I identified and added all stakeholders and component owners affected by this change as reviewers
  • I tested both happy and unhappy path of the functionality
  • I have made corresponding changes to the documentation

@trunk-io Trunk.io
Copy link

trunk-io bot commented Nov 29, 2024 •
edited
Loading

⏱️ 2h 12m total CI duration on this PR
Slowest 15 Jobs Cumulative Duration Recent Runs
execution-performance / single-node-performance 23m 🟩 
forge-e2e-test / forge 14m 🟩 
rust-move-tests 14m 🟩 
rust-move-tests 13m 🟩 
rust-move-tests 12m 🟩 
rust-move-tests 9m ⬜ 
rust-cargo-deny 9m 🟩 🟩 🟩 🟩 🟩 
rust-move-tests 7m ⬜ 
check-dynamic-deps 6m 🟩 🟩 🟩 🟩 🟩 
rust-doc-tests 6m 🟩 
test-target-determinator 5m 🟩 
execution-performance / test-target-determinator 4m 🟩 
check 4m 🟩 
general-lints 2m 🟩 🟩 🟩 🟩 🟩 
semgrep/ci 2m 🟩 🟩 🟩 🟩 🟩 

settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io

@0xjunha
[cli] Make chunk size configurable for chunked publish
50ed04e 
* Added a new `chunk_size` option to `ChunkedPublishOption`
* Updated `MAX_CHUNK_SIZE_IN_BYTES` to be of type `&str` with a value of `55000`
banool
banool approved these changes Nov 29, 2024
View reviewed changes
crates/aptos/src/common/types.rs Outdated Show resolved Hide resolved
crates/aptos/CHANGELOG.md Show resolved Hide resolved
gregnazario
gregnazario approved these changes Nov 29, 2024
View reviewed changes
Copy link
Contributor

@gregnazario gregnazario left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine, dport's comments should be addressed. Changelog should be 60 -> 50 k

And the default_value_t will make it easier but it's not the end of the world

@0xjunha
Fix clap args defaults and update the changelog
e86476d 
* Updated to use `default_value_t` instead of `default_value` with additional parser
* Added `CHUNK_SIZE_IN_BYTES` change info in the change log
@0xjunha 0xjunha enabled auto-merge (squash) November 29, 2024 17:39
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions
Copy link
Contributor

✅ Forge suite compat success on 010570d3b7aa20889fb5ad0e5b23800aa33f5634 ==> e86476d4525bbb511805793beb1a090338b14e6c

Compatibility test results for 010570d3b7aa20889fb5ad0e5b23800aa33f5634 ==> e86476d4525bbb511805793beb1a090338b14e6c (PR)
1. Check liveness of validators at old version: 010570d3b7aa20889fb5ad0e5b23800aa33f5634
compatibility::simple-validator-upgrade::liveness-check : committed: 17312.74 txn/s, latency: 1958.71 ms, (p50: 2100 ms, p70: 2100, p90: 2200 ms, p99: 2400 ms), latency samples: 559820
2. Upgrading first Validator to new version: e86476d4525bbb511805793beb1a090338b14e6c
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 7374.51 txn/s, latency: 3862.58 ms, (p50: 4300 ms, p70: 4500, p90: 4600 ms, p99: 4800 ms), latency samples: 137440
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 7637.62 txn/s, latency: 4248.56 ms, (p50: 4600 ms, p70: 4700, p90: 4800 ms, p99: 4800 ms), latency samples: 251580
3. Upgrading rest of first batch to new version: e86476d4525bbb511805793beb1a090338b14e6c
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 7340.41 txn/s, latency: 3894.38 ms, (p50: 4400 ms, p70: 4600, p90: 4700 ms, p99: 4800 ms), latency samples: 136500
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 7531.12 txn/s, latency: 4319.93 ms, (p50: 4700 ms, p70: 4800, p90: 4800 ms, p99: 5000 ms), latency samples: 245440
4. upgrading second batch to new version: e86476d4525bbb511805793beb1a090338b14e6c
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 10576.91 txn/s, latency: 2565.19 ms, (p50: 2700 ms, p70: 2800, p90: 3900 ms, p99: 4100 ms), latency samples: 183180
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 10874.56 txn/s, latency: 2889.01 ms, (p50: 2700 ms, p70: 2800, p90: 5300 ms, p99: 6500 ms), latency samples: 350600
5. check swarm health
Compatibility test for 010570d3b7aa20889fb5ad0e5b23800aa33f5634 ==> e86476d4525bbb511805793beb1a090338b14e6c passed
Test Ok

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions
Copy link
Contributor

✅ Forge suite framework_upgrade success on 010570d3b7aa20889fb5ad0e5b23800aa33f5634 ==> e86476d4525bbb511805793beb1a090338b14e6c

Compatibility test results for 010570d3b7aa20889fb5ad0e5b23800aa33f5634 ==> e86476d4525bbb511805793beb1a090338b14e6c (PR)
Upgrade the nodes to version: e86476d4525bbb511805793beb1a090338b14e6c
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1412.10 txn/s, submitted: 1415.29 txn/s, failed submission: 3.19 txn/s, expired: 3.19 txn/s, latency: 2127.42 ms, (p50: 1800 ms, p70: 2100, p90: 3300 ms, p99: 4800 ms), latency samples: 124080
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1279.71 txn/s, submitted: 1283.22 txn/s, failed submission: 3.51 txn/s, expired: 3.51 txn/s, latency: 2378.33 ms, (p50: 2100 ms, p70: 2400, p90: 3900 ms, p99: 5400 ms), latency samples: 116800
5. check swarm health
Compatibility test for 010570d3b7aa20889fb5ad0e5b23800aa33f5634 ==> e86476d4525bbb511805793beb1a090338b14e6c passed
Upgrade the remaining nodes to version: e86476d4525bbb511805793beb1a090338b14e6c
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1359.06 txn/s, submitted: 1361.71 txn/s, failed submission: 2.65 txn/s, expired: 2.65 txn/s, latency: 2191.59 ms, (p50: 1900 ms, p70: 2300, p90: 3300 ms, p99: 5500 ms), latency samples: 123080
Test Ok

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions
Copy link
Contributor

✅ Forge suite realistic_env_max_load success on e86476d4525bbb511805793beb1a090338b14e6c

two traffics test: inner traffic : committed: 14008.90 txn/s, latency: 2836.66 ms, (p50: 2700 ms, p70: 2700, p90: 3000 ms, p99: 3600 ms), latency samples: 5326500
two traffics test : committed: 100.02 txn/s, latency: 2252.29 ms, (p50: 2000 ms, p70: 2200, p90: 2300 ms, p99: 16900 ms), latency samples: 1780
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 2.292, avg: 1.172", "ConsensusProposalToOrdered: max: 0.305, avg: 0.292", "ConsensusOrderedToCommit: max: 0.404, avg: 0.379", "ConsensusProposalToCommit: max: 0.695, avg: 0.671"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.79s no progress at version 2088006 (avg 0.20s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 15.74s no progress at version 2088002 (avg 14.63s) [limit 16].
Test Ok

@0xjunha 0xjunha merged commit 261a185 into aptos-labs:main Nov 29, 2024
87 of 92 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@graphite-app graphite-app[bot] graphite-app[bot] left review comments

@wrwg wrwg wrwg approved these changes

@banool banool banool approved these changes

@gregnazario gregnazario gregnazario approved these changes

@davidiw davidiw Awaiting requested review from davidiw davidiw is a code owner

@movekevin movekevin Awaiting requested review from movekevin movekevin is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@0xjunha @gregnazario @banool @wrwg